-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moving to async/await: lib/Runner.js #464
Conversation
It used not to show, but just out of coincidence.
Just checked Travis log, I found this:
Am I doing something wrong? |
@lydell this is a potential downside of the better error handling we have rolled out. We do no longer get stack traces for errors like these. @sravanth-chebrolu it looks like somewhere you are returning an object containing a |
@harrysarson Flow should catch type errors. EDIT: Which it does.
@sravanth-chebrolu I recommend running Flow and ESLint and Pretter in your editor! |
Yeah I found the error, it was a stupid mistake, I forgot the |
Weird.. tests pass for Nodejs v15, but fail for the rest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures are because you need to put more of the function calls within the try catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I see the mistakes, I've learnt so much about promises from just this PR..
Should we merge #465? I don't understand why the checks are still failing |
@chebro The checks are failing because ESLint fails to parse
|
Will do this 👍🏽
Should I accept all incoming changes? |
Yes, that’s the plan once #465 is merged into master. (Note: There are one or two |
merge: lydell:improve-elm-test.js -> chebro:async-await
This is my first merge (with a conflict) on github, hope I did it right 🤞 |
I'll just wait until your branch is merged then. I can still hard reset if it comes to it.. |
CI is green 👍 marking as blocked as I cannot review due to all the changes pulled in from the other PR? |
Yes, I misunderstood what @lydell said, my bad. |
I am happy to merge this before #465 if it gets untangled. Equally I am very happy to wait :) |
Fixes #211 Addresses #442 (comment) This PR makes a bunch of improvements to elm-test.js: - Properly validate CLI arguments: - Error on unknown flags. - Validate flag values early (seed and fuzz must be numbers, report must be console/json/junit). - Default flag values early, instead of in code generation or even in Elm. - elm-test.js is now 100% definitions, except the last line which is `main();` to kick things off. Previously, it was difficult to understand in what order things executed. There was some top-level code, a bunch of function definitions, some more top-level code, more functions, and so on. Now, you can follow the call stack from `main()`. Also, this means that there are way fewer global variables – functions now get passed what they depend on. - Use async/await where it helps (in preparation for merge conflicts with #464 – it should be possible to just pick this PR’s code for every conflict in elm-test.js). - Improved help message. Also, add some aliases, such as `-h`, for showing help. I find it super annoying when CLI tools make it difficult to find the help – which is the key to all other commands and flags. - Various cleanups. Help before: ``` Usage: elm-test init # Create example tests Usage: elm-test install PACKAGE # Like `elm install PACKAGE`, except it installs to "test-dependencies" in your elm.json Usage: elm-test TESTFILES # Run TESTFILES, for example tests/**/*.elm Usage: elm-test [--compiler /path/to/compiler] # Run tests Usage: elm-test [--seed integer] # Run with initial fuzzer seed Usage: elm-test [--fuzz integer] # Run with each fuzz test performing this many iterations Usage: elm-test [--report json, junit, or console (default)] # Print results to stdout in given format Usage: elm-test [--version] # Print version string and exit Usage: elm-test [--watch] # Run tests on file changes ``` Intermediate help (outdated): ``` elm-test init Create example tests elm-test Run tests in the tests/ folder elm-test TESTFILES Run TESTFILES, for example "src/**/*Tests.elm" elm-test install PACKAGE Like `elm install PACKAGE`, except it installs to "test-dependencies" in your elm.json Options: --compiler /path/to/compiler Use a custom path to an Elm executable. Default: elm --seed INT Run with a previous fuzzer seed. Default: A random seed --fuzz INT Run with each fuzz test performing this many iterations. Default: 100 --report json --report junit --report console Print results to stdout in the given format. Default: console --version Print version and exit --watch Run tests on file changes ``` [commander](https://github.com/tj/commander.js) help: ``` Usage: elm-test [options] [globs...] elm-test Run tests in the tests/ folder elm-test "src/**/*Tests.elm" Run tests in files matching the glob Options: --compiler <path> Use a custom path to an Elm executable (default: elm) --seed <int> Run with a previous fuzzer seed (default: 68917302767402) --fuzz <int> Run with each fuzz test performing this many iterations (default: 100) --report <json|junit|console> Print results to stdout in the given format (default: "console") --watch Run tests on file changes (default: false) --version Print version and exit -h, --help Show help Commands: init Create example tests install <package> Like `elm install package`, except it installs to "test-dependencies" in your elm.json make [globs...] Check files matching the globs for compilation errors help [command] Show help ```
You are free to go for it @chebro! |
Aye! I'm a little busy this week, will start as soon as I can. |
No worries! Take your time (amongst other things of ci is broken so we can't merge until that is fixed anyway). |
Hmm, there seem to be some conflicts now, should I pull the master branch into this? |
Yeah! Don't worry about the commit history looking nice because I will squash when I merge |
Also note that this PR is not needed anymore if #475 is merged. Sorry! |
Alright, we can close this then. I'm glad that I got to take away something I previously didn't know about promises from this PR. Good luck :) |
Thanks @chebro, as they say "code is the easy part". Most the valuable work in open source is exploring the design space and working out what code you actually need, that is to say: PRs that do not get merged are no less valuable to a project as those that do. Till next time ... |
This makes the watcher faster and more stable! As a bonus, this made the code much more organized. - Concentrate all `process.exit` in elm-test.js (except one in Install.js where I found no clean solution). Besides making the code easier to follow, it allows the watcher to continue running (and recover) even if there are errors. - Don’t crash/stop running tests if files are removed. - Pick up new Elm files added. - Fixes #225 We already had code for waiting for the previous run to finish before starting the next run, but it queued up every single file change/add/remove event and ran that many runs one after the other. Now, for all watch events during a run we run _once_ after the current run for all of them. We also sleep 100 ms at the start of each run to catch more events happening close by (such as when adding/removing a whole folder of tests). (Note: We used to sleep at least 500 ms via the [awaitWriteFinish](https://github.com/paulmillr/chokidar#performance) chokidar option.) I’ve also tested manually that running elm-format on save doesn’t cause double test runs: #194 - Fixes #355 We now find the closest elm.json upwards in the folder hierarchy. - Resolves #453 (comment) The watcher now listens for changes to elm.json and properly recalculates things. I thought that I would have to recalculate some things only when elm.json changes for performance, but after making the globbing faster there was no need so we simply rebuild everything on each run. - Closes #463 and closes #464 I can’t find any more places to use async/await. - elm.json is now decoded properly in one place rather than here and there and just being `any`. We now have 0 explicit `any` type annotations! - elm-test.js now only contains CLI logic and subcommand dispatching. The code for running tests have been moved to RunTests.js and to FindTests.js (which used to be called Runner.js). - Improved .flowconfig that is stricter. Closes #478 - And lots of little code improvements along the way! I only touched things that I needed to change anyway in order to not make the diff balloon too much. I’ve done some manual testing on Windows, Mac and Linux to make sure things work like they should.
This PR attempts to close #463